Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make SubscriptionValue public #869

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make SubscriptionValue public #869

wants to merge 2 commits into from

Conversation

liam923
Copy link

@liam923 liam923 commented Apr 8, 2022

SubscriptionValue is seemingly needlessly private at the moment, and it is the only implementer of LeafAction that is private. This pr makes it part of the public api.

I have found this to be necessary for the use case of implementing subscriptions with a materialized schema. Without SubscriptionValue being public, the only way to make a resolver that can return a stream is via Field.subs. Thus, creating a fake field and then taking its resolver (Field.subs(...).resolver) is an unfortunate consequence of SubscriptionValue being private when using a materialized schema.

@yanns
Copy link
Contributor

yanns commented Apr 8, 2022

Thanks for the PR.
Do you think we could add a test to show this use-case? Would https://github.com/sangria-graphql/sangria/blob/main/modules/derivation/src/test/scala/sangria/streaming/StreamSpec.scala be a good place for that?

@liam923
Copy link
Author

liam923 commented Apr 8, 2022

Thanks for the PR. Do you think we could add a test to show this use-case? Would https://github.com/sangria-graphql/sangria/blob/main/modules/derivation/src/test/scala/sangria/streaming/StreamSpec.scala be a good place for that?

I went ahead and added a test as suggested. Unfortunately, it is a bit more involved that I had initially realized. Field.subs also adds a SubscriptionField tag to the field's tags, and without it, sangria raises a match error at at sangria.execution.Resolver.$anonfun$collectActionsPar$1(Resolver.scala:713). This pr thus likely requires creating an AstSchemaResolver that can add tags or modifying execution code to not require the tag.

@liam923
Copy link
Author

liam923 commented Apr 8, 2022

Or an even better solution would probably be creating some sort of SubscriptionFieldResolver to complement FieldResolver, which could deal with the tagging.

yanns
yanns approved these changes Apr 23, 2022
View reviewed changes
Copy link
Contributor

@yanns yanns left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the test! It's looking good and helps understanding the use-case.

@yanns
Copy link
Contributor

yanns commented Apr 23, 2022

I'm still trying to understand what is possible and what not.
And I'm not sure it's possible to add tags on the AST level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yanns yanns yanns approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@liam923 @yanns